Harden against undefined index in _load_script_textdomain_from_src() and add types for wp_parse_url()#11690
Conversation
…n _load_script_textdomain_from_src()
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| if ( ! $src_url ) { | ||
| return false; | ||
| } | ||
| $src_url['path'] ??= ''; |
There was a problem hiding this comment.
This is the crux for fixing the undefined index error.
There was a problem hiding this comment.
should we cover these bail-outs with unit tests?
There was a problem hiding this comment.
Yeah, that would be idea. I've added full coverage in b230450
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
@manzoorwanijk Please review. |
| } | ||
|
|
||
| $path = untrailingslashit( $path ); | ||
| if ( $path ) { |
There was a problem hiding this comment.
Conditional added because the $path can still be false here since $wp_textdomain_registry->get() returns string|false.
|
|
||
| // If the source is not from WP. | ||
| if ( false === $relative ) { | ||
| if ( ! is_string( $relative ) ) { |
There was a problem hiding this comment.
Since apply_filters() can return anything, safer to check for a string specifically than for equality with false.
There was a problem hiding this comment.
Pull request overview
This PR aims to harden script translation path resolution against edge cases in URL parsing (preventing undefined index / invalid offsets) and to improve static analysis by adding PHPStan typing for wp_parse_url() and its helper.
Changes:
- Adds guards/defaults in
_load_script_textdomain_from_src()to avoid undefined index access when working with parsed URLs. - Adjusts handling of unresolved relative paths to be more type-safe (
! is_string( $relative )). - Adds PHPStan conditional return types and array-shape annotations for
wp_parse_url(),_get_component_from_parsed_url_array(), and_wp_translate_php_url_constant_to_key().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/wp-includes/l10n.php |
Adds URL-parse hardening and path handling tweaks for script/module translation resolution. |
src/wp-includes/http.php |
Introduces PHPStan parameter/return typing for URL parsing helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…content URL Co-authored-by: Copilot Autofix powered by AI <[email protected]>
|
The addition of PHPStan types here (cf. Core-64238) ensures that all of the scenarios are accounted for which could cause errors. |
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks good to me. I just wonder whether we should add new test cases for the new bailout paths.
| if ( ! $src_url ) { | ||
| return false; | ||
| } | ||
| $src_url['path'] ??= ''; |
There was a problem hiding this comment.
should we cover these bail-outs with unit tests?
| * @return string|false The JSON-encoded translated strings on success, false otherwise. | ||
| */ | ||
| function _load_script_textdomain_from_src( string $handle, string $src, string $domain, string $path, bool $is_module ) { | ||
| /** @var WP_Textdomain_Registry $wp_textdomain_registry */ |
There was a problem hiding this comment.
Something that globally fix in #11692. Can we leave this for this PR
…om_src() Cover the three new short-circuit branches added in 8f38b36 (unparseable $src, unparseable content_url(), non-string $relative filter return) plus the $src_url['path'] default added in bfe0eca. Each test uses a pre_load_script_translations spy to assert the early-return branch was the one actually taken, rather than the result coincidentally matching the fallback-path behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Collapse the data-provider variants (null/true/array/int) into a single test using `__return_null` — all four hit the same one-line guard, so one case is enough to catch a regression. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Replace the bespoke by-ref-closure spy (not used elsewhere in tests/phpunit/) with the idiomatic MockAction helper. Keep the spy only on test_unparseable_content_url_returns_false, where the early return is the only thing distinguishing the new branch from the fallback path tail-end load_script_translations(false, ...) call. The other two tests do not need a spy: removing the early return for unparseable src triggers the "Automatic conversion of false to array" deprecation, and reverting the is_string guard triggers a TypeError in str_ends_with() -- both surface as test failures already. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…_src()`. This guards against an undefined index warning being raised when a script or script module is registered with a URL that lacks a path component. This also adds full PHPStan type definitions for `wp_parse_url()`, ensuring that the `_load_script_textdomain_from_src()` function has no PHPStan errors at rule level 10. Developed in #11690 Follow-up to r62278. Props westonruter, manzoorwanijk, mukesh27. See #65015, #64238. git-svn-id: https://develop.svn.wordpress.org/trunk@62293 602fd350-edb4-49c9-b593-d223f7449a82
…_src()`. This guards against an undefined index warning being raised when a script or script module is registered with a URL that lacks a path component. This also adds full PHPStan type definitions for `wp_parse_url()`, ensuring that the `_load_script_textdomain_from_src()` function has no PHPStan errors at rule level 10. Developed in WordPress/wordpress-develop#11690 Follow-up to r62278. Props westonruter, manzoorwanijk, mukesh27. See #65015, #64238. Built from https://develop.svn.wordpress.org/trunk@62293 git-svn-id: http://core.svn.wordpress.org/trunk@61573 1a063a9b-81f0-0310-95a4-ce76da25c4cd
See Trac comment for full description of issue: https://core.trac.wordpress.org/ticket/65015#comment:47
Trac ticket: https://core.trac.wordpress.org/ticket/65015
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Opus 4.7
Used for: PHPStan type research
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.